-
Notifications
You must be signed in to change notification settings - Fork 666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add callback support in settings and tools #590
Conversation
Change callbacks to be lists of functions rather than just functions Enhance tests
Co-authored-by: James Braza <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice -- looks good to me, I'm wondering if this could create serialization issues for the Settings
object, but that would be on the user as implemented here.
@pytest.mark.flaky(reruns=3, only_rerun=["AssertionError", "EmptyDocsError"]) | ||
@pytest.mark.asyncio | ||
async def test_agent_sharing_state( | ||
agent_test_settings: Settings, subtests: SubTests | ||
agent_test_settings: Settings, subtests: SubTests, callback_type: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be callback_type: str | None
gather_evidence_initialized_callback = AsyncMock() | ||
gather_evidence_completed_callback = AsyncMock() | ||
|
||
callbacks = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it really matters, but it's more future proof to use callbacks.update
here instead of redefining
Adds callback support to Settings
Tools can invoke these callbacks
I wrote this in a way so that the callbacks are immutable, future flexibility, and direct.
There will likely be more tools using this infrastructure in the future.